-
Notifications
You must be signed in to change notification settings - Fork 83
Handle quote comment deletion with Reject activity #2460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Introduces a new 'quote' comment type, updates interaction handling to extract and process quote links from content, and enhances reply detection to recognize quote-inline patterns. Includes comprehensive tests for quote extraction and reply identification.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Split the validation in add_comment() to separately check for valid comment data and the presence of 'inReplyTo'. This improves code clarity and ensures early return if comment data is invalid.
Introduces a 'display_style' property to comment type definitions, distinguishing between 'facepile' and 'comment' display styles. Adds helper methods to check if a comment type should be shown as a facepile and to retrieve all facepile types, enabling more flexible comment rendering.
Eliminated the 'display_style' property from comment type definitions and removed related methods for determining and retrieving facepile types. This simplifies comment type handling by no longer distinguishing between facepile and comment display styles.
Introduces comprehensive PHPUnit tests for the esc_hashtag function, including a data provider with various input cases, filter hook behavior, HTML escaping, and handling of quoted strings. These tests ensure correct hashtag formatting, proper escaping, and extensibility via filters.
Co-authored-by: Konstantin Obenland <[email protected]>
Implements detection and handling of quote activities using properties like quote, quoteUrl, quoteUri, and _misskey_quote. Updates interaction logic, reply/quote checks, and tests to support new quote formats and ensure correct comment type assignment.
Updated from_array method to avoid converting keys that start with an underscore to snake_case. This preserves the original format for special or private keys.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Introduces getter and setter methods for quote, quoteUrl, quoteUri, and _misskey_quote properties to support FEP-044f and platform-specific quoting features.
Replaces the use of the translation function for 'description' fields in activity types (Reposts, Likes, Quotes) with plain strings. This change may be intended to simplify or standardize these descriptions, possibly because translation is not needed for these fields.
Corrects the condition for converting camelCase to snake_case in Generic_Object by checking for underscore prefix using str_starts_with. Adds and expands PHPUnit tests for handling quote and underscore-prefixed properties in Base_Object, including round-trip set/get tests.
Refactored the function is_activity_quote to is_quote_activity for improved naming consistency. Updated all references and related tests to use the new function name.
Corrects the condition to convert camelCase to snake_case only for keys not prefixed with '_', ensuring proper key formatting in the Generic_Object class.
Changed @Covers annotations in class-test-generic-object.php to use the fully qualified class name Activitypub\Activity\Generic_Object for improved clarity and accuracy in test coverage reporting.
The 'quote' comment type is now included alongside 'like' and 'repost' in the Jetpack integration. This expands supported ActivityPub comment types.
Adds logic to send a Reject activity when a quote comment is deleted, revoking previously accepted QuoteRequest permissions. Updates Inbox to support querying by activity type and object ID, and adds comprehensive tests for quote comment deletion scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds functionality to send a Reject activity when a quote comment is deleted, properly revoking previously accepted QuoteRequest permissions. It enhances the Inbox collection with a new querying capability and includes comprehensive test coverage for various deletion scenarios.
Key changes:
- Implements quote comment deletion handling with Reject activity transmission
- Updates Inbox to store instrument URL as object_id for QuoteRequest activities, enabling efficient lookups
- Adds fallback reconstruction when original QuoteRequest is not found in inbox
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| includes/handler/class-quote-request.php | Adds handle_quote_delete() method to process quote comment deletions and send Reject activities, with inbox lookup and fallback logic |
| includes/collection/class-inbox.php | Implements get_by_type_and_object() query method and modifies object_id storage logic to use instrument URL for QuoteRequest activities |
| tests/phpunit/tests/includes/handler/class-test-quote-request.php | Adds four comprehensive test cases covering successful deletion with Reject, regular comment deletion, missing metadata handling, and inbox retrieval verification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* | ||
| * For activities with an 'instrument' property (e.g., QuoteRequest), we store | ||
| * the instrument URL as the object_id. This allows efficient querying by instrument. | ||
| * For all other activities, we store the object URL as before. | ||
| */ | ||
| if ( $activity->has( 'instrument' ) && $activity->get_instrument() ) { | ||
| $object_id = object_to_uri( $activity->get_instrument() ); | ||
| } else { | ||
| $object_id = object_to_uri( $activity->get_object() ); | ||
| } |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider documenting the side effect of this change on existing code that may query _activitypub_object_id meta. For QuoteRequest activities, the _activitypub_object_id now stores the instrument URL instead of the object URL. This is a breaking change in the meta structure. While this is necessary for the new feature, adding a note in the comment about backwards compatibility or migration considerations would be helpful. For instance, any existing code that queries by object URL for QuoteRequest activities will no longer find them.
| $post_id = $wpdb->get_var( | ||
| $wpdb->prepare( | ||
| "SELECT p.ID | ||
| FROM {$wpdb->posts} p | ||
| INNER JOIN {$wpdb->postmeta} pm1 ON p.ID = pm1.post_id AND pm1.meta_key = '_activitypub_activity_type' | ||
| INNER JOIN {$wpdb->postmeta} pm2 ON p.ID = pm2.post_id AND pm2.meta_key = '_activitypub_object_id' | ||
| WHERE p.post_type = %s | ||
| AND pm1.meta_value = %s | ||
| AND pm2.meta_value = %s | ||
| ORDER BY p.ID DESC | ||
| LIMIT 1", | ||
| self::POST_TYPE, | ||
| $activity_type, | ||
| $object_id | ||
| ) | ||
| ); |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance concern: the query uses two INNER JOINs on postmeta without ensuring indexes exist. WordPress postmeta table has indexes on meta_key but not composite indexes. This query could be slow on sites with large postmeta tables. Consider:
- Adding a note about potential performance implications
- Suggesting database indexes if this becomes a frequently called method
- Exploring alternative approaches like caching or using a more efficient query structure if performance becomes an issue
The query pattern is:
INNER JOIN {$wpdb->postmeta} pm1 ON p.ID = pm1.post_id AND pm1.meta_key = '_activitypub_activity_type'
INNER JOIN {$wpdb->postmeta} pm2 ON p.ID = pm2.post_id AND pm2.meta_key = '_activitypub_object_id'This results in two table scans on postmeta which could be expensive.
| $inbox_item = Inbox::get_by_type_and_object( 'QuoteRequest', $instrument_url ); | ||
|
|
||
| if ( ! \is_wp_error( $inbox_item ) && $inbox_item instanceof \WP_Post ) { | ||
| $activity_object = \json_decode( $inbox_item->post_content, true ); |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for JSON decode failure. If $inbox_item->post_content contains invalid JSON, json_decode() will return null, causing the fallback path to be used silently. Consider checking for JSON decode errors explicitly:
$activity_object = \json_decode( $inbox_item->post_content, true );
if ( JSON_ERROR_NONE !== \json_last_error() ) {
$activity_object = null;
}This makes it clear when the fallback is used due to corrupted data versus a missing inbox item.
| $activity_object = \json_decode( $inbox_item->post_content, true ); | |
| $activity_object = \json_decode( $inbox_item->post_content, true ); | |
| if ( JSON_ERROR_NONE !== \json_last_error() ) { | |
| $activity_object = null; | |
| } |
| /** | ||
| * Get an inbox item by activity type and object ID. | ||
| * | ||
| * This is useful for finding specific activity types (like QuoteRequest) | ||
| * by their object identifier. For QuoteRequest activities, the object_id | ||
| * is the instrument URL (the quote post). | ||
| * | ||
| * @param string $activity_type The activity type (e.g., 'QuoteRequest'). | ||
| * @param string $object_id The object identifier to search for. | ||
| * | ||
| * @return \WP_Post|\WP_Error The inbox item or WP_Error if not found. |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation could be more specific about what "object_id" represents for different activity types. The current documentation only mentions QuoteRequest, but doesn't clarify what object_id means for other activities. Consider adding:
* For QuoteRequest activities, the object_id is the instrument URL (the quote post).
* For other activities, the object_id is the URL of the activity's object.
This would make it clearer to future developers how this method should be used.
| * For QuoteRequest activities, the inbox stores the instrument URL | ||
| * in _activitypub_object_id, so we can query by that. | ||
| */ | ||
| $activity_object = null; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assignment $activity_object = null; is unnecessary since it's immediately used in a condition at line 150 that checks for null/empty. This line can be removed as $inbox_item result will be checked, and $activity_object is only assigned if the condition passes. If the condition fails, the fallback block at line 155 assigns a value to $activity_object, so the initialization to null serves no purpose.
| $activity_object = null; |
| if ( ! $post || ! $post->post_author ) { | ||
| return; | ||
| } |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete validation: the check ! $post->post_author will pass for posts with post_author = "0" (string zero) which is truthy in PHP, but will fail for legitimate posts with author ID 0. However, more importantly, this doesn't validate that the post author is a valid ActivityPub-enabled user. Consider using Actors::get_by_id() to validate the user exists and is an ActivityPub actor before proceeding, similar to what's done in queue_reject() at line 258.
| if ( ! $post || ! $post->post_author ) { | |
| return; | |
| } | |
| if ( ! $post || ! isset( $post->post_author ) ) { | |
| return; | |
| } | |
| $actor = Actors::get_by_id( $post->post_author ); | |
| if ( ! $actor ) { | |
| return; | |
| } |
| 'type' => 'QuoteRequest', | ||
| 'actor' => $comment->comment_author_url, | ||
| 'object' => \get_permalink( $post_id ), | ||
| 'instrument' => $instrument_url, |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider including 'published' field in the fallback reconstruction for consistency with the test expectations. The test test_delete_uses_inbox_item creates a QuoteRequest with a 'published' field (line 646), and the comment at line 714 states "verify the reject object is a QuoteRequest with proper structure". While not required for the Reject to function, including all original fields in the fallback would maintain better parity with the retrieved inbox version and make the fallback more complete.
| 'instrument' => $instrument_url, | |
| 'instrument' => $instrument_url, | |
| 'published' => gmdate( 'c' ), |
| if ( $activity->has( 'instrument' ) && $activity->get_instrument() ) { | ||
| $object_id = object_to_uri( $activity->get_instrument() ); | ||
| } else { | ||
| $object_id = object_to_uri( $activity->get_object() ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a situation where $object_id = object_to_uri( $activity->get_instrument() ?: $activity->get_object() ); could work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe... I was not sure if we can simply always prioritize instrument over object or if I also have to check for a *Request type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like at least Friendica has a different interpretation of what instrument is 💪
| $activity_type, | ||
| $object_id | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be a WP_Query call instead? It might help with caching
| $activity_object = null; | ||
| $inbox_item = Inbox::get_by_type_and_object( 'QuoteRequest', $instrument_url ); | ||
|
|
||
| if ( ! \is_wp_error( $inbox_item ) && $inbox_item instanceof \WP_Post ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if ( ! \is_wp_error( $inbox_item ) && $inbox_item instanceof \WP_Post ) { | |
| if ( $inbox_item instanceof \WP_Post ) { |
| // Fallback: If inbox item not found, reconstruct from available data. | ||
| if ( ! $activity_object ) { | ||
| // Log when fallback is used for monitoring/debugging. | ||
| \do_action( 'activitypub_quote_request_inbox_not_found', $instrument_url, $post_id ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs action docs. Do we even need this action?
Adds logic to send a Reject activity when a quote comment is deleted, revoking previously accepted QuoteRequest permissions. Updates Inbox to support querying by activity type and object ID, and adds comprehensive tests for quote comment deletion scenarios.
Proposed changes:
Other information:
Testing instructions:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Send a Reject activity when a quote comment is deleted, revoking previous quote permissions and ensuring consistent inbox handling.